-
Notifications
You must be signed in to change notification settings - Fork 19
Web IDL support 2/N: response JSON serialization #155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: json
Are you sure you want to change the base?
Conversation
41fbcff to
68c78a6
Compare
|
@msirringhaus — Following up on your review comment on PR #138 regarding I investigated the WebAuthn Level 3 specification and found that
This matches the behavior observed on demo.yubico.com, which always returns So the current implementation in this PR is correct — we always include
Both cases result in the same output per the spec, since This comment was written by GitHub Copilot (Claude) operating on @AlfioEmanueleFresta's instructions. |
68c78a6 to
d3c31bb
Compare
fbb40b2 to
39cca52
Compare
This adds the inverse of JSON request parsing - serializing WebAuthn responses back to JSON format per WebAuthn Level 3 specification. Changes: - Add WebAuthnIDLResponse trait for response-to-JSON conversion - Add RegistrationResponseJSON and AuthenticationResponseJSON models - Implement to_json() for MakeCredentialResponse and Assertion - Refactor requests to store challenge/origin/cross_origin separately - Add client_data_hash() and client_data_json() helper methods - Update all examples and tests to use new request structure - Add unit tests for response serialization
d3c31bb to
d97c80d
Compare
msirringhaus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Some minor comments and questions inline.
| impl ClientData { | ||
| pub fn hash(&self) -> Vec<u8> { | ||
| /// Returns the canonical JSON representation of the client data. | ||
| pub fn to_json_bytes(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be more versatile to make this to_json() and return a String. Those who need the bytes can then call into_bytes() on the result.
| hash: Vec::from(challenge), | ||
| challenge: Vec::from(challenge), | ||
| origin: "example.org".to_string(), | ||
| cross_origin: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any tests where cross_origin is not None?
| self.client_data().hash() | ||
| } | ||
|
|
||
| pub fn client_data_json(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above. More so here: The naming client_data_json() would lead me to expect a String.
| let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); | ||
|
|
||
| // Verify required fields | ||
| assert!(parsed.get("id").is_some()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to also check the contents of the fields, not just if they are Some()?
|
|
||
| /// JSON output format options. | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub enum JsonFormat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional to 'reimplement' this, instead of exposing a serde_json-Value, which the user can then use and format according to whatever serde_json offers?
I don't have a strong preference for either way, just wanting to understand the reasoning better, if there is any.
| request: &Self::Context, | ||
| ) -> Result<Self::InnerModel, ResponseSerializationError> { | ||
| // Get credential ID from attested credential data | ||
| let credential_id_bytes = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the GetAssertion-case regarding crential IDs.
| }, | ||
| authenticator_attachment: None, | ||
| client_extension_results, | ||
| r#type: "public-key".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this for now, as we do it in lots of places, but somehow my engineering mind would feel more at ease, if we could make this a transparent enum instead of a hardcoded string in a bunch of places, to avoid typos. But this doesn't have to be in this PR.
| let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); | ||
|
|
||
| // Verify required fields | ||
| assert!(parsed.get("id").is_some()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with GetAssertion: Can we check the contents as well for those that are easy/obvious?
Summary
This PR is the second in a series (follows #138) and adds JSON serialization for WebAuthn responses - the inverse of the existing JSON request parsing functionality.
Changes
New Response Serialization Infrastructure
WebAuthnIDLResponsetrait - Providesto_json()andto_inner_model()methods for converting responses to JSON formatJsonFormatenum - Supports both minified and prettified JSON outputResponseSerializationError- Error type for serialization failuresNew JSON Response Models (per WebAuthn Level 3 spec)
RegistrationResponseJSON- ForMakeCredentialResponseserializationAuthenticationResponseJSON- ForAssertionserializationAuthenticatorAttestationResponseJSON- Attestation response detailsAuthenticatorAssertionResponseJSON- Assertion response detailsAuthenticationExtensionsClientOutputsJSON- Extension output serializationRequest Structure Refactoring
Refactored
MakeCredentialRequestandGetAssertionRequestto enable client data generation on-the-fly:hash: Vec<u8>withchallenge: Vec<u8>- Store raw challenge instead of pre-computed hashorigin: Stringfield - Store origin explicitlycross_origin: Option<bool>field - Optional cross-origin flagclient_data: ClientDatafield - No longer needed as separate fieldclient_data()- BuildsClientDatainternallyclient_data_hash()- Computes SHA-256 hash on demandclient_data_json()- Returns JSON bytes for response serializationImplementation Details
WebAuthnIDLResponseforMakeCredentialResponsewith full attestation object CBOR serializationWebAuthnIDLResponseforAssertionwith signature and authenticator data handlingcredProps,hmacCreateSecret,hmacGetSecret,largeBlob,prfSerializederives to attestation statement types for CBOR encodingUpdated Files
client_data_hash()Testing
webauthn_json_hid.rsexample to demonstrate JSON response outputExample Usage